chore(code): bump diffs package version#2648
Conversation
|
React Doctor could not complete this scan.
Reviewed by React Doctor for commit |
adboio
added
the
Stamphog
Graphite App
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates denied this PR because it modifies dependency/toolchain files (package.json files and pnpm-lock.yaml), which require human review. Additionally, the rename of DIFF_METRICS.fileGap to spacing is a behavioral API contract change that needs a reviewer to confirm no callers are broken.
stamphog
Bot
removed
the
Stamphog
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/core/src/code-review/diffAnnotations.test.ts:83-100
**Prefer parameterised tests for boolean toggle behaviour**
The two new `buildCommentMergedOptions` cases only differ on the `hasOpenComment` flag and the expected value of `enableLineSelection`/`enableGutterUtility`. Expressing them as a single `it.each` table (e.g. `[false, true, true], [true, false, false]`) would make it easier to see the invariant and add future cases without duplicating the setup.
Reviews (1): Last reviewed commit: "chore(code): bump diffs package version" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
|
|
||
| describe("buildCommentMergedOptions", () => { | ||
| it("routes both selection end and gutter utility clicks to the handler", () => { | ||
| const handler = () => {}; | ||
| const merged = buildCommentMergedOptions(undefined, false, handler); | ||
| expect(merged.enableLineSelection).toBe(true); | ||
| expect(merged.enableGutterUtility).toBe(true); | ||
| expect(merged.onLineSelectionEnd).toBe(handler); | ||
| expect(merged.onGutterUtilityClick).toBe(handler); | ||
| }); | ||
|
|
||
| it("disables selection and gutter utility while a comment is open", () => { | ||
| const merged = buildCommentMergedOptions(undefined, true, () => {}); | ||
| expect(merged.enableLineSelection).toBe(false); | ||
| expect(merged.enableGutterUtility).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The two new buildCommentMergedOptions cases only differ on the hasOpenComment flag and the expected value of enableLineSelection/enableGutterUtility. Expressing them as a single it.each table (e.g. [false, true, true], [true, false, false]) would make it easier to see the invariant and add future cases without duplicating the setup.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/code-review/diffAnnotations.test.ts
Line: 83-100
Comment:
**Prefer parameterised tests for boolean toggle behaviour**
The two new `buildCommentMergedOptions` cases only differ on the `hasOpenComment` flag and the expected value of `enableLineSelection`/`enableGutterUtility`. Expressing them as a single `it.each` table (e.g. `[false, true, true], [true, false, false]`) would make it easier to see the invariant and add future cases without duplicating the setup.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
adboio
force-pushed
the
06-12-chore_code_bump_diffs_package_version
branch
from
550e7b7 to
c8bf992
Compare
1 participant
Problem
diffs package is a bit out of date; want to upgrade before testing a pre-release version
Changes
bump pierre/diffs version
How did you test this?
manually
Automatic notifications